Add akka.hosting #148

Arkatufus · 2022-11-13T14:13:11Z

This is an expansion of #146 with added Akka.Hosting support

…thChecks

wesselkranenborg · 2022-11-16T14:46:36Z

@Arkatufus : I think it looks great and you did some nice improvements here. I have only one concent and that's about the following comment I put on my draft PR: This persistence liveness check actor needs to be singleton in a cluster. The persistenceId will otherwise try to write data from all nodes under the same persistenceId to the same storage. That will cause conflicts resulting in the fact that your cluster will become unstable now and then.

Or was this an issue because I did wire up the healthcheck actors myself instead of using the ActorExtension for that?

wesselkranenborg · 2022-11-16T14:47:21Z

If you could create a beta package for this, I could test this in our solution and see if this works as expected.

Arkatufus · 2022-11-16T14:57:22Z

This persistence liveness check actor needs to be singleton in a cluster. The persistenceId will otherwise try to write data from all nodes under the same persistenceId to the same storage.

That is a bit problematic since the persistence healthcheck supossed to check that the current node can actually access the backend storage and properly read-write from it and not coupled with clustering/remoting.

I think it is better if we change the persistence id instead, tying it to the current cluster node. Problem with this approach is to figure out a way to create a unique persistence id that is guaranteed to not collide with anything else that is trying to access the storage backend.

wesselkranenborg · 2022-11-16T21:10:53Z

I think it is better if we change the persistence id instead, tying it to the current cluster node.

That's indeed also a good solution. Right now this is the persistenceId: https://github.com/petabridge/akkadotnet-healthcheck/blob/dev/src/Akka.HealthCheck.Persistence/AkkaPersistenceLivenessProbe.cs#L175

Can we not append the nodeIP to that if the node is part of an Akka.Cluster? I think you're safe then.

wesselkranenborg · 2022-11-16T21:12:44Z

You only then need to make sure that you not only remove latest -1 snapshot and messages but all messages because if nodes leave/join the cluster this might become an unexpected mess of orphaned data.

https://github.com/petabridge/akkadotnet-healthcheck/blob/dev/src/Akka.HealthCheck.Persistence/AkkaPersistenceLivenessProbe.cs#L204

Arkatufus · 2022-11-16T21:22:59Z

Well, if we make sure that every incarnation of the probe have unique persistence ids, there won't be any mess because each of them would have their own snapshots

wesselkranenborg · 2022-11-16T22:28:24Z

Guid.NewGuid() 😉

Arkatufus · 2022-11-17T20:16:54Z

I guess we can use Guid. I'm a bit leery with Guid because I've heard that the Guid algorithm only guarantee non-collision if it is generated by the same computer, the collision probability increases If multiple computers generates Guid

Aaronontheweb · 2022-12-01T16:08:57Z

That is a bit problematic since the persistence healthcheck supossed to check that the current node can actually access the backend storage and properly read-write from it and not coupled with clustering/remoting.

That's correct.

Aaronontheweb · 2022-12-01T16:10:05Z

I guess we can use Guid. I'm a bit leery with Guid because I've heard that the Guid algorithm only guarantee non-collision if it is generated by the same computer, the collision probability increases If multiple computers generates Guid

Guid collisions are exceedingly rare so I wouldn't worry about that. I'd prefix the entity name with something like healthcheck-{Guid}.

Aaronontheweb

Have some minor cleanup to do - but let's merge this in, make available to users, get some feedback. Can you also make a separate PR with installation / configuration steps for IHealthCheck integration?

Aaronontheweb · 2022-12-01T16:11:45Z

src/Akka.HealthCheck.Cluster/ClusterLivenessProbe.cs

-            _livenessStatus = livenessStatus;
+            var selfMember = _cluster.State.Members.FirstOrDefault(m => m.Address == _cluster.SelfAddress);
+            var isUp = selfMember is { Status: MemberStatus.Up or MemberStatus.WeaklyUp };
+            _livenessStatus = isUp ? new LivenessStatus(true) : DefaultClusterLivenessStatus;


Aaronontheweb · 2022-12-01T16:13:31Z

src/Akka.HealthCheck.Hosting/AkkaHostingExtensions.cs

+{
+    public static class AkkaHostingExtensions
+    {
+        public static IServiceCollection WithAkkaHealthCheck(this IServiceCollection services)


This adds the ASP.NET healthchecks (IHealthCheck)

Aaronontheweb · 2022-12-01T16:18:39Z

src/Akka.HealthCheck.IHealthCheckExample/Akka.HealthCheck.IHealthCheckExample.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Akka.Cluster.Hosting" Version="0.5.1" />


Should derive from common.props / Directory.Build.props

Aaronontheweb · 2022-12-01T16:19:40Z

src/Akka.HealthCheck.Hosting/Services/AkkaReadinessService.cs

+
+namespace Akka.HealthCheck.Hosting.Services
+{
+    public class AkkaReadinessService : IHealthCheck


Ought to add some XML-DOC here

Aaronontheweb · 2022-12-01T16:20:07Z

...ka.HealthCheck.IHealthCheckExample/snapshots/snapshot-Akka.HealthCheck-23-638039442589894743

@@ -0,0 +1 @@
+hit2%System.String, System.Private.CoreLib


Should probably exclude this file from git

Aaronontheweb · 2022-12-06T18:31:26Z

@Arkatufus guess we just need to address the comments on the review in order to merge + release this?

Aaronontheweb · 2022-12-06T19:31:53Z

@Arkatufus is this ready for review?

Arkatufus · 2022-12-09T22:20:08Z

No, not yet, sorry, was busy with Akka.Hosting.
Let me give it another once over to make sure it is review worthy.

Arkatufus · 2022-12-13T17:13:03Z

The persistence healthcheck is still problematic, i'm implementing it with GUID here, and the healthcheck will litter the database with random healthcheck data since the id is generated on each healhtcheck actor instantiation

Arkatufus · 2022-12-13T17:15:06Z

it wasn't a problem before because it will use the same id over and over, since we can't find a per actor discriminator that can survive actor/actor system restart, this could be a problem...

Arkatufus · 2022-12-13T17:16:52Z

We can't use a static counter because there is no guarantee that other nodes would not have another healthcheck running, creating an id collision

Arkatufus · 2022-12-13T17:19:01Z

an ideal solution would be to have a persisted unique id for each actor system node without relying on clustering

# Conflicts: # src/common.props

Aaronontheweb · 2022-12-15T21:01:23Z

@Arkatufus let me know when this is ready to take out of draft stage

Arkatufus · 2022-12-21T19:01:09Z

@Aaronontheweb This should be ready for review now

Arkatufus · 2022-12-21T19:01:40Z

The GUID problem is still unresolved

Aaronontheweb

LGTM - need to validate the nuget publication locally (or check what the build server produced.) Don't want to publish any sample projects and need to make sure that the correct README.md files are included.

Aaronontheweb · 2022-12-22T14:33:43Z

src/Akka.HealthCheck.Hosting.Web/README.md

@@ -0,0 +1,181 @@
+# Akka.HealthCheck.Hosting.Web


Make sure this README is what ends up inside the Akka.HealthCheck.Hosting.Web NuGet package when it's pushed. Include some of the other boilerplate about Akka.NET and Akka.Hosting that a user might need to know too.

wesselkranenborg and others added 5 commits November 2, 2022 16:32

Rename Akkka in example projectname to Akka

005ff2d

Rename also namespace in current example

1508f77

Example implementation of using Akka.Healthchecks together with IHeal…

e7ab338

…thChecks

Merge branch 'dev' into dev

0bef97f

Add Akka.Hosting

c931b4a

Arkatufus marked this pull request as draft November 13, 2022 14:13

Arkatufus mentioned this pull request Nov 13, 2022

Sample implementation of Akka.HealthCheck in combination with IHealthCheck #146

Closed

Merge branch 'dev' into add_akka.hosting

ae3cdbd

Aaronontheweb requested changes Dec 1, 2022

View reviewed changes

Merge branch 'dev' into add_akka.hosting

c75a749

Arkatufus mentioned this pull request Dec 6, 2022

Add Support for Microsoft.Extensions.Diagnostics.IHealthCheck? akkadotnet/Akka.Hosting#23

Closed

Use common.props for Akka.Hosting version

eab1c35

Add XML-DOCs

032974b

Add GUID to persistence healthchecks

e6843d0

Merge branch 'dev' into add_akka.hosting

f51f0dc

# Conflicts: # src/common.props

Arkatufus added 6 commits December 17, 2022 04:59

Refactor API

2e5f3b1

Code cleanup, remove accidental inclusion of local snapshot files

d6216e0

Merge branch 'dev' into add_akka.hosting

db52c12

Refactor service to probe

51e531c

Add custom IProbeProvider sample project

129b389

Add documentation

d38944f

Arkatufus marked this pull request as ready for review December 21, 2022 19:00

Revert changes to persistence probe, moved to its own PR.

33c0f5e

Aaronontheweb approved these changes Dec 22, 2022

View reviewed changes

Aaronontheweb mentioned this pull request Dec 22, 2022

need to validate the nuget publication before 1.0 ships #155

Closed

Aaronontheweb merged commit 4869fb6 into petabridge:dev Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add akka.hosting #148

Add akka.hosting #148

Arkatufus commented Nov 13, 2022

wesselkranenborg commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 16, 2022 •

edited

Loading

wesselkranenborg commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 17, 2022

Aaronontheweb commented Dec 1, 2022

Aaronontheweb commented Dec 1, 2022

Aaronontheweb left a comment

Aaronontheweb Dec 1, 2022

Aaronontheweb Dec 1, 2022

Aaronontheweb Dec 1, 2022

Aaronontheweb Dec 1, 2022

Aaronontheweb Dec 1, 2022

Aaronontheweb commented Dec 6, 2022

Aaronontheweb commented Dec 6, 2022

Arkatufus commented Dec 9, 2022 •

edited

Loading

Arkatufus commented Dec 13, 2022 •

edited

Loading

Arkatufus commented Dec 13, 2022

Arkatufus commented Dec 13, 2022

Arkatufus commented Dec 13, 2022

Aaronontheweb commented Dec 15, 2022

Arkatufus commented Dec 21, 2022

Arkatufus commented Dec 21, 2022

Aaronontheweb left a comment

Aaronontheweb Dec 22, 2022

Add akka.hosting #148

Add akka.hosting #148

Conversation

Arkatufus commented Nov 13, 2022

wesselkranenborg commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 16, 2022 • edited Loading

wesselkranenborg commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 16, 2022

wesselkranenborg commented Nov 16, 2022

Arkatufus commented Nov 17, 2022

Aaronontheweb commented Dec 1, 2022

Aaronontheweb commented Dec 1, 2022

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb Dec 1, 2022

Choose a reason for hiding this comment

Aaronontheweb Dec 1, 2022

Choose a reason for hiding this comment

Aaronontheweb Dec 1, 2022

Choose a reason for hiding this comment

Aaronontheweb Dec 1, 2022

Choose a reason for hiding this comment

Aaronontheweb Dec 1, 2022

Choose a reason for hiding this comment

Aaronontheweb commented Dec 6, 2022

Aaronontheweb commented Dec 6, 2022

Arkatufus commented Dec 9, 2022 • edited Loading

Arkatufus commented Dec 13, 2022 • edited Loading

Arkatufus commented Dec 13, 2022

Arkatufus commented Dec 13, 2022

Arkatufus commented Dec 13, 2022

Aaronontheweb commented Dec 15, 2022

Arkatufus commented Dec 21, 2022

Arkatufus commented Dec 21, 2022

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb Dec 22, 2022

Choose a reason for hiding this comment

Arkatufus commented Nov 16, 2022 •

edited

Loading

Arkatufus commented Dec 9, 2022 •

edited

Loading

Arkatufus commented Dec 13, 2022 •

edited

Loading